fix(instant): make forced conversion sheet non-dismissible#125
Merged
Conversation
When `forceInstantUserConversion` is enabled, the sign-in sheet must be non-dismissible until the user actually adds an identifier. Previously the sheet defaulted to dismissible via swipe, tap-outside, and any Hub close message, which left customers with large populations of unconverted instant users. - BottomSheetViewController gains an `isUserDismissalDisabled` flag that disables swipe-to-dismiss and tap-outside-to-dismiss at presentation, and that the Hub's `can_touch_background_to_dismiss` message cannot re-enable. - `hideBottomSheet` and `HubViewController.hide()` honor the same lock, so Hub-dispatched `closeHubViewController`/`signOut` messages cannot close the sheet either. - InstantUsers observes the post-conversion auth-level transition and releases the lock once the user is no longer `.instant`, so the standard post-auth auto-close path still runs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reviewer's GuideImplements a forced-conversion lock for the Rownd sign-in bottom sheet so that, when configured, instant users cannot dismiss the sheet until they add a non-instant identifier, while ensuring the lock is automatically released after successful conversion and does not leak across sessions. Sequence diagram for forced conversion lock on instant user sign-in sheetsequenceDiagram
actor InstantUser
participant InstantUsers
participant Rownd
participant BottomSheetViewController
participant HubViewController
InstantUser->>InstantUsers: authLevelPublisher (isAuthenticated=true, authLevel=instant)
InstantUsers->>Rownd: requestSignInForcedConversion(signInOptions)
Rownd->>BottomSheetViewController: isUserDismissalDisabled = true
Rownd->>Rownd: requestSignIn(signInOptions)
Rownd->>BottomSheetViewController: presentAsBottomSheet(controller, behavior)
BottomSheetViewController->>BottomSheetViewController: viewWillAppear()
BottomSheetViewController->>BottomSheetViewController: setCanTouchDimmingBackgroundToDismiss(false)
par User attempts to dismiss
InstantUser->>BottomSheetViewController: hideBottomSheet()
BottomSheetViewController->>BottomSheetViewController: [isUserDismissalDisabled]
BottomSheetViewController-->>InstantUser: ignore hideBottomSheet
InstantUser->>HubViewController: closeHubViewController
HubViewController->>HubViewController: hide()
HubViewController->>HubViewController: [bottomSheetController.isUserDismissalDisabled]
HubViewController-->>InstantUser: ignore hide
InstantUser->>BottomSheetViewController: canTouchDimmingBackgroundToDismiss(true)
BottomSheetViewController->>BottomSheetViewController: [isUserDismissalDisabled]
BottomSheetViewController-->>InstantUser: ignore enable
end
InstantUser->>InstantUsers: authLevelPublisher (authLevel=verified)
InstantUsers->>Rownd: releaseForcedConversionLock()
Rownd->>BottomSheetViewController: isUserDismissalDisabled = false
HubViewController->>HubViewController: hide()
HubViewController->>BottomSheetViewController: hideBottomSheet()
BottomSheetViewController->>BottomSheetViewController: dismiss()
BottomSheetViewController-->>HubViewController: completion
HubViewController-->>InstantUser: dismiss(animated=true)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Review Summary by QodoMake forced instant user conversion sheet non-dismissible until conversion
WalkthroughsDescription• Prevents dismissal of forced-conversion sign-in sheet via swipe, tap-outside, or Hub close messages • Adds isUserDismissalDisabled flag to BottomSheetViewController for locking sheet during conversion • Implements lock release mechanism when user transitions from instant to verified auth level • Maintains programmatic dismissal capability after successful authentication Diagramflowchart LR
A["User enters instant state"] -->|requestSignInForcedConversion| B["Lock enabled<br/>isUserDismissalDisabled=true"]
B -->|Disable swipe/tap-outside| C["Sheet non-dismissible"]
C -->|Block Hub close messages| D["User must convert"]
D -->|Auth level transitions<br/>to verified/unverified| E["Lock released<br/>isUserDismissalDisabled=false"]
E -->|Auto-close proceeds| F["Sheet dismissed"]
File Changes1. Sources/Rownd/Rownd.swift
|
Code Review by Qodo
1. Lock not applied live
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+183
to
+187
| @MainActor | ||
| internal static func requestSignInForcedConversion(_ signInOptions: RowndSignInOptions?) { | ||
| inst.bottomSheetController.isUserDismissalDisabled = true | ||
| requestSignIn(signInOptions) | ||
| } |
There was a problem hiding this comment.
1. Lock not applied live 🐞 Bug ≡ Correctness
requestSignInForcedConversion() sets isUserDismissalDisabled = true, but the actual bottom-sheet dismissibility is only changed in BottomSheetViewController.viewWillAppear, so if the Hub is already presented the sheet remains user-dismissible despite the lock.
Agent Prompt
### Issue description
Forced conversion sets a flag (`isUserDismissalDisabled`) but does not reliably apply it to an already-presented bottom sheet. The only place that calls `setCanTouchDimmingBackgroundToDismiss(false)` based on the flag is `BottomSheetViewController.viewWillAppear`, which won’t run again if the bottom sheet is already on-screen.
### Issue Context
`displayViewControllerOnTop` intentionally avoids re-presenting the bottom sheet when it’s already presented, so updating the lock must update the existing `sheetController` immediately.
### Fix Focus Areas
- Sources/Rownd/Rownd.swift[183-187]
- Sources/Rownd/Rownd.swift[431-447]
- Sources/Rownd/Views/BottomSheetViewController.swift[32-65]
### Suggested fix
Implement a `didSet` on `isUserDismissalDisabled` (or a dedicated method) that, when toggled, immediately calls `sheetController?.setCanTouchDimmingBackgroundToDismiss(...)` if `sheetController` already exists. This ensures the lock takes effect even when the Hub sheet is already presented.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
There was a problem hiding this comment.
Addressed in 9ea56d0. isUserDismissalDisabled now has a didSet that pushes through to the live sheetController via setCanTouchDimmingBackgroundToDismiss(...), so toggling the flag after presentation takes effect immediately. Same path covers your second comment — releasing the lock restores the Hub's most recent dismissibility request (tracked in a separate hubRequestedCanTouchToDismiss so we don't unconditionally force true on unlock).
mhamann
commented
May 12, 2026
Contributor
Author
mhamann
left a comment
mhamann
left a comment
There was a problem hiding this comment.
Targeted, mostly contained fix. Two concerns I'd resolve before relying on this in production:
Correctness
- A close path bypasses the lock —
HubViewController.viewWillDisappear(line 145, unchanged in this PR) callshostController.dismiss(animated: true)directly, sidesteppinghideBottomSheet. Any system or programmatic event that disappears the Hub VC will close the sheet regardless of the lock. See line comment onHubViewController.swift:186. - Race between the post-auth 1.5s
hide()timer andUserData.fetchpropagatingauthLevel. IfauthLevelis still.instantwhen the timer fires,hide()is blocked; the lock-release later fires from the user-data update but nothing re-invokeshide(), so the sheet can stay open after a successful conversion. See line comment onInstantUsers.swift:56.
Design
.guestreleases the lock alongside.verified/.unverified. That may or may not be what the customer wants — guest is still anonymous-ish. Worth a deliberate decision (line comment onInstantUsers.swift:56).
Nits
- No unit tests added.
InstantUsershas enough state-machine surface now (thehasTriggeredConversiongate, the dual transitions) to warrant a small test using a fake store. BottomSheetViewController.hideBottomSheetdrops the completion when locked. Today the only caller isHubViewController.hide()and we block above that path, so it doesn't fire — but it's a latent footgun (line comment onBottomSheetViewController.swift:89).logger.debugfor the blocked-dismissal messages is fine, but operators debugging "sheet won't close" reports will need to enable debug logging. Considerlogger.log/info.
Landing recommendation: address the viewWillDisappear bypass and the close-race (a few lines each); merge after that.
| } | ||
|
|
||
|
|
||
| if bottomSheetController.isUserDismissalDisabled { |
Contributor
Author
There was a problem hiding this comment.
Bug — lock bypass. The new check guards hide(), but HubViewController.viewWillDisappear at line 145 (unchanged here) directly calls:
hostController.dismiss(animated: true)…on the bottom-sheet host. That path neither checks isUserDismissalDisabled nor goes through hideBottomSheet, so any system event that disappears the Hub VC will tear the sheet down despite the lock. Easy reproductions: presenting another modal over the Hub, deep-linking to a flow that swaps the sheet, or anything calling dismiss on a parent.
Suggest adding the same early-return on the lock at line 141, or routing all dismissals through hideBottomSheet so there's one chokepoint.
Contributor
Author
There was a problem hiding this comment.
Leaving as-is per offline discussion. The viewWillDisappear path requires something to disappear the Hub VC from outside the SDK, which our customers shouldn't be doing on the forced-conversion flow. Worth revisiting if we see field reports of escape via this path.
|
|
||
| // User has converted to a non-instant auth level (verified, unverified, guest). | ||
| // Release the lock so the Hub's post-success auto-close can proceed. | ||
| if self.hasTriggeredConversion && authLevel != .instant && authLevel != .unknown { |
Contributor
Author
There was a problem hiding this comment.
Race — sheet can stay open after successful conversion.
HubWebViewController dispatches the tokens on .authentication then schedules hide() 1.5s later:
store.dispatch(store.state.auth.onReceiveAuthTokens(
AuthState(accessToken: authMessage.accessToken, refreshToken: authMessage.refreshToken)
))
...
DispatchQueue.main.asyncAfter(deadline: .now() + 1.5) { [weak self] in
if initialJsFunctionArgsAsJson == self?.jsFunctionArgsAsJson {
self?.hubViewController?.hide()
}
}The authLevel update comes from UserData.fetch(), not from the token dispatch. On a slow network (or any 1.5s+ fetch latency) the timeline is:
- t=0: tokens dispatched,
authLevelstill.instant - t=1.5s:
hide()fires → lock still set → blocked, sheet stays open - t=2s+:
UserData.fetch()returns,authLevelflips to.verified - This sink fires, lock released — but nothing re-invokes
hide(), so the sheet is now unlocked-but-still-open
The user sees a sheet that won't close on its own. They can swipe-dismiss it (because the lock is released), but that's an unexpected UX.
Suggest re-triggering dismissal when releasing the lock, e.g.:
Rownd.releaseForcedConversionLock()
// Re-attempt the post-auth close that may have been blocked by the lock.
(Rownd.inst.bottomSheetController.controller as? HubViewProtocol)?.hide()
subscriber.unsubscribe()(Or add a releaseForcedConversionLockAndDismiss() helper that does both.)
Contributor
Author
There was a problem hiding this comment.
Fixed in 9ea56d0. Rownd.releaseForcedConversionLock now re-invokes hide() on the active HubViewProtocol after clearing the flag, so if the post-auth hide() timer fired during the locked window and was blocked, we close the sheet as soon as authLevel propagates and the lock releases. Guarded on wasLocked so calling release with no lock held is a no-op.
|
|
||
| // User has converted to a non-instant auth level (verified, unverified, guest). | ||
| // Release the lock so the Hub's post-success auto-close can proceed. | ||
| if self.hasTriggeredConversion && authLevel != .instant && authLevel != .unknown { |
Contributor
Author
There was a problem hiding this comment.
Question — .guest releases the lock. Going .instant → .guest isn't really "adding an identifier" in the spirit of forceInstantUserConversion. If a customer's Hub config exposes a "continue as guest" path, the user could escape the forced flow without converting. Is that the intended behavior, or should the release condition be authLevel == .verified || authLevel == .unverified only?
Contributor
Author
There was a problem hiding this comment.
Keeping current behavior (.guest releases the lock alongside .verified/.unverified). The contract of forceInstantUserConversion is "force the user out of the auto-assigned .instant state" — .guest is a deliberate user choice and treating it as a successful exit avoids stranding users when a customer's Hub config exposes a guest path. If a customer wants stricter enforcement, the right knob is disabling guest auth in Hub config.
| } | ||
|
|
||
| public func hideBottomSheet(_ completion: (() -> Void)? = nil) { | ||
| if isUserDismissalDisabled { |
Contributor
Author
There was a problem hiding this comment.
Nit — completion silently dropped. When locked, the completion parameter is never invoked. Today the only caller is HubViewController.hide(), which is itself blocked one level up, so this doesn't fire in practice. But the signature still promises "call me back when the sheet is hidden," and a future caller could quietly wedge if they depend on that. Worth a doc comment at minimum, or have callers check isUserDismissalDisabled themselves and skip passing the completion.
Contributor
Author
There was a problem hiding this comment.
Leaving as-is. The only caller passing a completion (HubViewController.hide()) is itself guarded by the lock check one level up, so the completion is never registered when the inner block fires. Worth revisiting if a second caller appears.
Addresses review feedback from PR #125: - BottomSheetViewController: `isUserDismissalDisabled` now has a `didSet` that updates the live `sheetController` so the lock takes effect on an already-presented sheet. Track the Hub's last-requested dismissibility separately so releasing the lock restores it rather than unconditionally re-enabling (Qodo #1, #2). - Rownd: `releaseForcedConversionLock` now re-invokes `hide()` on the active HubViewProtocol so a post-auth auto-close that lost the race against `UserData.fetch` propagating `authLevel` still closes the sheet. Guarded on the prior locked state so unlocking a non-held lock is a no-op (avoids side effects in tests). - Add `Rownd._bottomSheetIsLocked` internal accessor for tests. - Tests: cover lock engagement on `.instant`, release on `.verified`, and the once-per-session gate after release. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 tasks
mhamann
added a commit
that referenced
this pull request
May 12, 2026
* fix(instant): make forced conversion sheet non-dismissible When `forceInstantUserConversion` is enabled, the sign-in sheet must be non-dismissible until the user actually adds an identifier. Previously the sheet defaulted to dismissible via swipe, tap-outside, and any Hub close message, which left customers with large populations of unconverted instant users. - BottomSheetViewController gains an `isUserDismissalDisabled` flag that disables swipe-to-dismiss and tap-outside-to-dismiss at presentation, and that the Hub's `can_touch_background_to_dismiss` message cannot re-enable. - `hideBottomSheet` and `HubViewController.hide()` honor the same lock, so Hub-dispatched `closeHubViewController`/`signOut` messages cannot close the sheet either. - InstantUsers observes the post-conversion auth-level transition and releases the lock once the user is no longer `.instant`, so the standard post-auth auto-close path still runs. * fix(instant): apply lock live and re-trigger close on release Addresses review feedback from PR #125: - BottomSheetViewController: `isUserDismissalDisabled` now has a `didSet` that updates the live `sheetController` so the lock takes effect on an already-presented sheet. Track the Hub's last-requested dismissibility separately so releasing the lock restores it rather than unconditionally re-enabling (Qodo #1, #2). - Rownd: `releaseForcedConversionLock` now re-invokes `hide()` on the active HubViewProtocol so a post-auth auto-close that lost the race against `UserData.fetch` propagating `authLevel` still closes the sheet. Guarded on the prior locked state so unlocking a non-held lock is a no-op (avoids side effects in tests). - Add `Rownd._bottomSheetIsLocked` internal accessor for tests. - Tests: cover lock engagement on `.instant`, release on `.verified`, and the once-per-session gate after release. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When
Rownd.config.forceInstantUserConversionis enabled, the SDK now actually forces the sign-in sheet to stay open until the user adds an identifier. Previously the sheet defaulted to dismissible by swipe, tap-outside, and any Hub-dispatched close message — so customers with large populations of instant users would see the prompt and dismiss it without converting.Reported impact: ~8,000 active instant users that never converted, on at least one customer.
What changed
BottomSheetViewControllergains anisUserDismissalDisabledflag. When set before presentation,viewWillAppearcallssetCanTouchDimmingBackgroundToDismiss(false)after the LBBottomSheet is created (disables both swipe-to-dismiss and tap-outside-to-dismiss). The flag is reset onviewDidDisappear. The existingcanTouchDimmingBackgroundToDismiss(_:)now refuses re-enable calls from the Hub while the lock is active.hideBottomSheetandHubViewController.hide()both honor the same lock, so the Hub'scloseHubViewControllerandsignOutmessages cannot close the sheet either.Rownd.requestSignInForcedConversion(_:)/Rownd.releaseForcedConversionLock()— internal helpers to set/clear the lock.InstantUsersnow keeps its subscription alive past the initial trigger. When the user'sauthLeveltransitions out of.instant(to.verified,.unverified, or.guest), it releases the lock so the Hub's post-success auto-close (hide()1.5s after the.authenticationmessage) proceeds normally..unknownis treated as a transient state and does not release.No Hub-side change required
The fix is iOS-only. Existing customers don't need to update their Hub configuration — flipping
forceInstantUserConversionon the iOS side is sufficient.Test plan
forceInstantUserConversion = false(default): sheet behavior is unchanged — swipe, tap-outside, and Hub close messages all dismiss as before.forceInstantUserConversion = true:.instantstate.closeHubViewControllermessage) does not dismiss.authLeveltransitions to non-.instant.landmarksscheme) on an iOS 18 simulator with the flag set — verified locally.Notes
BottomSheetViewControllerand self-resets onviewDidDisappear, so it cannot leak across app launches.🤖 Generated with Claude Code
Summary by Sourcery
Enforce non-dismissible sign-in bottom sheet for instant users when forced conversion is enabled, and automatically release the lock after successful conversion.
New Features:
Enhancements: